fix(transport): cancel in-flight request on streamable-HTTP client disconnect (#857)#967
Open
ameyypawar wants to merge 3 commits into
Open
Conversation
…sconnect (modelcontextprotocol#857) When a streamable-HTTP client closes its TCP connection while a tool handler is still running, the request-wise SSE response stream is dropped, but nothing fired the per-request cancellation token. Only a natural response completion or an explicit notifications/cancelled cancelled it, so a long-running handler ran to completion after the client had already disconnected. Wrap the request-wise response stream in a drop-guard. When the stream is dropped before completing (client disconnect), it injects a synthetic notifications/cancelled for the request, reusing the existing cancellation path so the service fires the handler's RequestContext::ct. Streams that end normally (response delivered, or a reconnection handoff) do not cancel. Adds a regression test covering both the disconnect-cancels and the normal-completion paths. No public API change.
…elcontextprotocol#857) Add the `[[test]]` entry so the new integration test is only built when its required features (server, transport-streamable-http-server, reqwest) are enabled, matching the other streamable-http test targets.
… ::new (modelcontextprotocol#857) - Wrap the synthetic-cancellation construction to match `cargo +nightly fmt`. - `ServerInfo` is `#[non_exhaustive]`; construct it in the test with `ServerInfo::new(..)` instead of a struct literal. Verified locally: `cargo +nightly fmt --all -- --check` clean and the regression test passes (disconnect cancels the in-flight request).
github-actions
Bot
added
T-dependencies
DaleSeo
reviewed
Jul 10, 2026
DaleSeo
left a comment
DaleSeo
left a comment
Member
There was a problem hiding this comment.
Thanks for the fix, @ameyypawar! I left a couple of comments.
| }); | ||
| Ok(futures::stream::iter(priming).chain(ReceiverStream::new(receiver.inner))) | ||
| let stream = futures::stream::iter(priming).chain(ReceiverStream::new(receiver.inner)); | ||
| Ok(CancelOnDisconnect::new(stream, handle.clone(), request_id)) |
Member
There was a problem hiding this comment.
Could we also handle client disconnects after a request-wise SSE stream has been resumed? resume() currently returns a plain ReceiverStream without the disconnect guard.
| // blocking. If the channel is momentarily full the request is not | ||
| // cancelled early, but the session keep-alive/idle timeout still reaps | ||
| // it — no worse than before this fix. | ||
| let _ = self.handle.event_tx.try_send(SessionEvent::ClientMessage { |
Member
There was a problem hiding this comment.
Could we make sure the disconnect cancellation isn’t silently dropped when the session event channel is full? Since the result of try_send is ignored, a busy session could still leave the handler running until cleanup.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #857.
Motivation and Context
When a client connected over
transport-streamable-http-servercloses its TCP connection while a tool handler is still running, the server-side future was not cancelled — it ran to completion, and the per-requestRequestContext::ctnever fired. Explicitnotifications/cancelledalready worked; only the raw TCP-disconnect case was affected.Root cause: the per-request cancellation token is only fired on a natural response (
Event::ToSink) or an explicitnotifications/cancelled. The request-wise SSE response stream is what observes the disconnect (the runtime drops it), but nothing wired that drop back to the token.What this does
Wraps the request-wise SSE response stream returned by
LocalSessionManager::create_streamin a small private drop-guard (CancelOnDisconnect):notifications/cancelledfor the request through the session's existing event channel. The service's existing cancellation path then fires the handler'sRequestContext::ct— exactly as an explicit client cancellation would.This reuses the existing cancellation machinery and adds no public API (the guard is private, and
create_streamstill returnsimpl Stream<Item = ServerSseMessage>).How Has This Been Tested?
Added
crates/rmcp/tests/test_streamable_http_disconnect_cancel.rs(2 tests):client_disconnect_cancels_in_flight_request: starts a long-running tool over streamable HTTP, drops the client connection mid-flight, and asserts the handler's cancellation token fires (it does, promptly).normal_tool_response_is_delivered: a normal, fully-read tool call still returns unchanged (the guard does not spuriously cancel or corrupt the response).Both pass locally. Also verified
cargo +nightly fmt --all -- --checkandcargo clippyare clean on the changed code.Breaking Changes
None. Additive; no wire or public-API change.
Types of changes
Checklist
Note
#857 has a comment from @loocor (2026-06-05, "I'd like to take this one"), but no PR followed in over a month, so I went ahead. Happy to defer or collaborate if they are still working on it.